Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(deps): update to React 19 #393

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

crgwbr
Copy link
Contributor

@crgwbr crgwbr commented Dec 19, 2024

No description provided.

package.json Outdated
Comment on lines 10 to 13
],
"overrides": {
"react": "^19.0.0"
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed since react-helmet-async doesn't officially support React 19 yet. But it seems to work fine. I'm a little concerned that it's a dead project, but I'll probably submit a MR to it anyway.

See staylor/react-helmet-async#239

@silviogutierrez
Copy link
Owner

@crgwbr : I love this, obviously, and want to move to React 19. But this is... pretty bold. Lot of things still not supporting React 19. Though it may be like react-helmet-async and they actually do just work fine.

Once tests pass I'll give it a go with my main project.

@crgwbr
Copy link
Contributor Author

crgwbr commented Dec 19, 2024

@silviogutierrez What would you think of getting rid of the helmet dependency entirely, in lieu of React 19's new metadata tag support? It'd involve some refactoring of how renderPage() works, but I think it'd be possible.

See https://react.dev/blog/2024/12/05/react-19#support-for-metadata-tags

@silviogutierrez
Copy link
Owner

Not opposed at all, I just have no idea how the tags are injected in the server side part. Given the app shell is rendered outside of React.

When 19 came out I scoured the docs and can't find a single reference. And yet it says it "works".

@silviogutierrez
Copy link
Owner

For the life of me, I cannot find a single example of how meta tags are "hoisted" on the server render. renderToString and its friends all return a strong that then goes inside your non-react-code app shell.

And yet the docs mention this works with SSR.

@silviogutierrez
Copy link
Owner

Maybe we have to move to this: https://react.dev/reference/react-dom/server/renderToReadableStream

@crgwbr
Copy link
Contributor Author

crgwbr commented Dec 31, 2024

@silviogutierrez Just pushed some prototype code for you to look at—native meta tag hoisting seems to work if we generate the app shell via React, rather than using string templating. The client side app still only rehydrates the part of the app within div#root. There's surely some bugs to workout here, but let me know your thoughts on the approach overall.

@silviogutierrez silviogutierrez added the snapshot Triggers a snapshot release to NPM and PyPI label Jan 11, 2025
@github-actions github-actions bot removed the snapshot Triggers a snapshot release to NPM and PyPI label Jan 11, 2025
Copy link
Owner

@silviogutierrez silviogutierrez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be working with my biggest project. Will do more testing and reviewing but leaving this here now. Great work!

"scripts": {
"postinstall": "patch-package"
},
"overrides": {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we go rid of helmet, do we still need this?

Does it cascade down to consumer projects? If so, we do want that.

packages/reactivated/src/shell.tsx Show resolved Hide resolved
@silviogutierrez
Copy link
Owner

Ok good news and bad news.

Turns out those quote issues are unrelated, so ignore them. I marked the comment as complete.

Bad news: useId is unstable with this approach. React wants an exact document tree on the server and the client, otherwise they won't match. And useId is used all over the place in React. So you'll get rehydration issues.

Try it, in your app just call useId and render it server side and client side. You'll get a mismatch.

This is due to our architecture change of rendering a full document on the server and just a root node on the client.
save.patch

Attached is a POC patch that fixes the issue, but it comes at significant cost. We now have hydration on the client include the shell. So the shell needs to be aware of a lot of reactivated internals that the library consumer was not bothered with.

Note also in this crude approach we suppress some hydration stuff. But it's pretty gnarly.

Options:

  1. Go back to react-helmet-async.
  2. Figure out a way to expose the concept of the shell to the client with a consolidated API.
  3. Encourage and move towards App or whatever being a full document that users now have to provide. But vite's code needs to be injected somehow.

I'm going to let this marinate and welcome your thoughts.

It does seem like React is very heavily encouraging full document handling, and all APIs are moving in that direction. So might be best to rip the bandaid.

Related issues (notably, HMR still works with the fix):

https://github.com/vercel/next.js/pull/31102/files
vitejs/vite-plugin-react#293
vitejs/vite-plugin-react#222

@silviogutierrez
Copy link
Owner

@crgwbr

@silviogutierrez
Copy link
Owner

Also I'm looking at this renderToPipeableStream

Seems like there are some parameters there that could be related and are meant for these situations. Maybe hydration takes them into account.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants